Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the current user's SDKMAN installation #718

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reitzig
Copy link
Contributor

@reitzig reitzig commented Nov 6, 2019

When su-ing to another user, environment variables carry over.
As a result, sdk will point to the installation of the original user,
not the one running the current shell.

Thus: Reset $SDKMAN_DIR if the directory it points at
belongs to a user other than the current one.

(Sorry, I didn't have the time to fulfill the prerequisites. I empathize, but that doesn't change my time prioritization.)

When `su`-ing to another user, environment variables carry over. 
As a result, `sdk` will point to the installation of the original user, 
not the one running the current shell.

Thus: Reset `$SDKMAN_DIR` if the directory it points at
belongs to a user other than the current one.
@reitzig reitzig changed the title Reset SDKMAN_DIR if it has wrong owner Use the current user's SDKMAN installation Nov 6, 2019
@@ -25,7 +25,7 @@ if [ -z "$SDKMAN_CANDIDATES_API" ]; then
export SDKMAN_CANDIDATES_API="@SDKMAN_CANDIDATES_API@"
fi

if [ -z "$SDKMAN_DIR" ]; then
if [ -z "$SDKMAN_DIR" ] || [ $(ls -ld "${SDKMAN_DIR}" | awk '{ print $3 }') != $(whoami) ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I do see the point, the implementation does feel a bit hacky to me.
Can we assign this expression to a clearly named variable? Can we unset said variable at the end of the script? Can we collapse the two conditional blocks into one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and of course, please refrain from using awk as this forces a hard dependency that will often not be met on very basic systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and of course, please refrain from using awk as this forces a hard dependency that will often not be met on very basic systems.

Which systems are that? The smallest I know comes with (an) awk:

$ docker run --rm alpine awk -version
BusyBox v1.30.1 (2019-06-12 17:51:55 UTC) multi-call binary.

Of course, BusyBox binaries are quite often not compatible with GNU ones... :/

That said, which dependencies are you comfortable with? For instance, I used to use the innocuous stat but it turns out that macOS and Ubuntu come with incompatible versions (FreeBSD-ish vs GNU-ish). ls + awk was the best I could come up with -- I'm open for suggestions!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cut command is preferable as it is packaged as part of gnu coreutils. You could try something like this:

ls -ld "${SDKMAN_DIR}" | cut -d" " -f3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I do see the point, the implementation does feel a bit hacky to me.

Do you have a proposal? I felt it's a pretty straight-forward implementation for "do we point at this user's installation?". Of course, it will fail with system installations, but IIRC that's not a use-case you support?

Can we assign this expression to a clearly named variable? Can we unset said variable at the end of the script?

Sure!

Can we collapse the two conditional blocks into one?

What do you mean? [ -z ... ] || [ ... ] into [ ... ]? No, I don't think so; [ 1 -eq 1 || 1 -eq 2 ] doesn't parse. [[ 1 -eq 1 || 1 -eq 2 ]] would work, but you use single brackets everywhere else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes I do think so. We use double square brackets throughout the sdkman codebase when dealing with complex expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cut command is preferable as it is packaged as part of gnu coreutils. You could try something like this:

ls -ld "${SDKMAN_DIR}" | cut -d" " -f3

This does fail on the current Alpine (BusyBox v1.30.1). :/

Then, maybe stat wrapped away in a function and distinguish flavors there? That's in coreutils, after all.

@reitzig
Copy link
Contributor Author

reitzig commented Nov 6, 2019

It occurs to me that SDKMAN_VERSION is similarly affected. Is there a reason against not setting it to $(cat ${SDKMAN_DIR}/var/version), always?

@smac89
Copy link
Contributor

smac89 commented Dec 21, 2020

To run a command as another user, don't use su, use sudo instead. By default, sudo will clear the current user's environment variables before starting the command.

sudo --user=<user> <cmd...>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants